-
-
Notifications
You must be signed in to change notification settings - Fork 632
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix rule graph nondeterminism due to undetected ambiguity #7379
Fix rule graph nondeterminism due to undetected ambiguity #7379
Conversation
…dep. Otherwise, we will not detect the case where the same rule can be satisfied by multiple parameter combinations.
…e if it consumes a provided Get param (possibly transitively).
Individual commits are useful to review independently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
let mut rules_by_kind: HashMap<EntryWithDeps, (usize, &Entry)> = HashMap::new(); | ||
for satisfiable_entry in &satisfiable_entries { | ||
// We prefer the non-ambiguous rule with the smallest set of Params, as that minimizes Node | ||
// identities in the graph and biases toward receiving values from dependencies (which do not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/identifies/selections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is "identities", which is accurate here... we refer to the Params
that a @rule
consumes as part of its "identity", because the same @rule
can be used with multiple sets of Params
, which might each (as seen in this ticket!) affect their behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have enough context for any meaningful comments. Will read through it tomorrow but would not like to block it.
…nested_raise` (because it was not installed).
Problem
Integration tests of
@console_rules
would frequently flake (as in #6782) with rules self-cycling in a peculiar way. Investigating the failure lead to the conclusion that theRuleGraph
was being constructed non-deterministically.The source of the non-determinism was that we were "simplifying" all instances of one
@rule
that used the same number ofParams
, and treating them interchangeably. But that simplification was subject to rust's randomized hash ordering, and so we would choose which of theParams
to use at random.After fixing that non-determinism, we also needed to solve the underlying ambiguities (there were two or three) that lead to there being multiple options to choose from.
Solution
GraphMaker::choose_dependency
to not simplify rules, and instead preserve all candidates with the current minimumParam
set size.Param
provided by aGet
are eligible to satisfy it. See the explanation in the test.Result
Unskips
@console_rule
integration tests, and adds a (previously flaky, now stable) unit test of the ambiguous case. Fixes #6782.